Define targetRef proto #2888

jaellio · 2023-08-08T23:06:10Z

Resolves #2885.

Part of Replacing WorkloadSelector with TargetRef for Waypoints

Adds a targetRef proto based on the policy attachment concept in Gateway API. Adds the targetRef field to AuthorizationPolicy, RequestAuthentication, Telemetry, and WasmPlugin protos.

Notable changes from design doc: (copied from this comment)

ProxyConfig will not have a targetRef
targetRef will only be valid for k8s gateways (not istio gateways) due to the complexity
Ambient policies will not have workload selectors in the root namespace
If a resource with a targetRef is applied to the root namespace it will not be applied mesh wide. Only to the resource it explicitly targets
No need for ReferenceGrant because of point 2; policies will be in the same ns as the workloads which will always be the same ns as the waypoints (for beta scope at least)

Remaining work:

Add examples to the AuthorizationPolicy, RequestAuthentication, Telemetry, and WasmPlugin protos once the control plane implementation is complete.

istio-testing · 2023-08-08T23:06:12Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

istio-policy-bot · 2023-08-08T23:06:13Z

😊 Welcome @jaellio! This is either your first contribution to the Istio api repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

jaellio · 2023-08-08T23:08:37Z

/test all

networking/v1beta1/proxy_config.proto

security/v1beta1/request_authentication.proto

MorrisLaw

great work!

type/target_ref.proto

MorrisLaw

Don't forget to add release notes as well.

type/target_ref.proto

MorrisLaw

lgtm

extensions/v1alpha1/wasm.proto

costinm

Left few comments - interesting that K8S spec doesn't seem to have a section in targetRef, but it does in parentRef in HttpRoute.

That's a pretty large problem for our policies - a gateway may have 100 hostnames and ports, having policy with targetRef=gateway implies the rules apply to all 100, and if users are not careful to add matches on hostname in the policy - bad things happen. Even if they do, it's pretty inefficient.

That could be worked around with gateway merging ( only one listener per gateway ), not sure if that got implemented.

extensions/v1alpha1/wasm.proto

networking/v1beta1/proxy_config.proto

releasenotes/notes/target-ref.yaml

security/v1/authorization_policy.proto

security/v1/request_authentication.proto

hzxuzhonghu · 2023-08-10T02:04:55Z

For targetRef = gateway or other kind of resource, how can we know which proxy should the policy apply to?

I think that will be a little hard.

There could be case one CR has tragetRef to another kind of resource like gateway, but ay t0 the gateway doesnot occur yet, how do we handle that?

I think ideally we need to record the reference relationships and trigger handling after the gateway occur, but that would be expensive. Or it could also triggerc a full xds like what we do now. Not sure how this intermediate dependency compared with direct dependency.

security/v1/authorization_policy.proto

keithmattix · 2023-08-10T14:55:18Z

There could be case one CR has tragetRef to another kind of resource like gateway, but ay t0 the gateway doesnot occur yet, how do we handle that?

I wasn't fully aware that Istio APIs didn't have status until pondering this question. IMO, this is a blocker for adopting policy attachment correctly:

In terms of status, it should be reasonably easy for a user to understand that everything is working - basically, as long as the targeted object exists, and the modifications are valid, the metaresource is valid, and this should be straightforward to communicate in one or two Conditions.

If we implement status, we can report "gateway.networking.k8s.io/Gateway my-gateway does not exist" as an "Invalid" condition. From there, if the Gateway is created afterwards, there's no extra xDS push required; the policy would be taken into account when building the first push context. If the reverse happens (Gateway is created before policy) then yes a full push should probably occur.

hzxuzhonghu · 2023-08-11T02:28:54Z

Ther could be some use case like ratelimiting, we need both targetRef and workload selector to apply the ratelimiting policy to a specific client workload.

keithmattix · 2023-08-11T18:18:02Z

Ther could be some use case like ratelimiting, we need both targetRef and workload selector to apply the ratelimiting policy to a specific client workload.

I'm a bit confused; why would targetRef or workloadSelector have anything to do with the client workload? Policy attaches to the producer service, no?

hzxuzhonghu · 2023-08-14T02:32:12Z

Ther could be some use case like ratelimiting, we need both targetRef and workload selector to apply the ratelimiting policy to a specific client workload.

I'm a bit confused; why would targetRef or workloadSelector have anything to do with the client workload? Policy attaches to the producer service, no?

The user need the limit th especific workload only. Nope, the policy can/should also work on consumer side.

extensions/v1alpha1/wasm.proto

security/v1/authorization_policy.proto

keithmattix · 2023-08-14T13:55:10Z

To my knowledge, Ambient only has producer waypoints/policy. Is that correct @linsun @howardjohn? That decision predates me

linsun · 2023-08-31T13:22:02Z

security/v1/authorization_policy.proto

+  // Optional. The targetRef specifies the waypoint the policy should be 
+  // applied to. The targeted resource specified will determine which 
+  // workloads the authorization policy applies to. The targeted resource
+  // must be a waypoint. The waypoint must be in the same namespace as
+  // the authorization policy.
+  //
+  // If not set, the policy is applied as defined by the selector.


Should we clarify if targetRef not set, it could mean namespace scoped waypoint?

I think this might be better captured in an example.

linsun · 2023-08-31T13:27:37Z

Hi folks (cc @keithmattix @hzxuzhonghu @howardjohn etc),

Wanted to share some inconsistency between namespace scoped authz policy vs workload centric authz policy. For namespace scoped authz policy, I can continue to use the same authz policy for sidecar and ambient workloads.

For workload scoped authz policy, I would have to create a new authz policy for ambient workloads (using targetRef) along with existing authz policy for sidecar as I start to migrate sidecar to ambient.

Is my understanding correct?

If so, what do you think of supporting worload selector and targetRef (as OR relationship but allow users to declare both) and Istio can apply to it based on data plane option where workload selector is only relevant for sidecar and targetRef is only relevant for ambient waypoints?

keithmattix · 2023-08-31T13:42:51Z

@linsun AuthZ policy in particular has some nuances, specifically in the context of the layer targeted authorization policy proposal that is awaiting approval. If possible, could we move forward with these API changes and hash out the implementation details in the authorization policy PR?

That being said, I think your suggestion matches the direction that @whitneygriffith is going on her implementation

howardjohn · 2023-08-31T14:25:04Z

Lin - no matter what, users will need to change their workload selectors for waypoints based on decisions prior to this (that policies apply to the waypoint, not backends). This PR doesn't change that.

…

On Thu, Aug 31, 2023 at 6:43 AM Keith Mattix II ***@***.***> wrote: @linsun <https://github.com/linsun> AuthZ policy in particular has some nuances, specifically in the context of the layer targeted authorization policy proposal that is awaiting approval. If possible, could we move forward with these API changes and hash out the implementation details in the authorization policy PR? — Reply to this email directly, view it on GitHub <#2888 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXNJXXL4XHQB75GU6GTXYCIGPANCNFSM6AAAAAA3JFGBBI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

linsun · 2023-08-31T18:06:59Z

Agreed, user will need to change, just trying to minimize changes for them.

@keithmattix @jaellio i am good with the PR overall. made a comment earlier: #2888 (comment)

Signed-off-by: Jackie Elliott <[email protected]>

ProxyConfig, and RequestAuthentication. Need more examples. Signed-off-by: Jackie Elliott <[email protected]>

Signed-off-by: Jackie Elliott <[email protected]>

comments. Added release note for targetRef. Signed-off-by: Jackie Elliott <[email protected]>

use of taretRef. Signed-off-by: Jackie Elliott <[email protected]>

Signed-off-by: Jackie Elliott <[email protected]>

examples. Signed-off-by: Jackie Elliott <[email protected]>

complete Signed-off-by: Jackie Elliott <[email protected]>

Signed-off-by: Jackie Elliott <[email protected]>

oneof to wasm plugin. Signed-off-by: Jackie Elliott <[email protected]>

Signed-off-by: Jackie Elliott <[email protected]>

targeted resource Signed-off-by: Jackie Elliott <[email protected]>

Signed-off-by: Jackie Elliott <[email protected]>

extensions/v1alpha1/wasm.proto

type/v1beta1/selector.proto

howardjohn · 2023-09-05T17:56:24Z

type/v1beta1/selector.proto

+// multiple child resources. The PolicyTargetReference will be used instead of
+// a WorkloadSelector in the RequestAuthentication, AuthorizationPolicy,
+// Telemetry, and WasmPlugin CRDs to target k8s Gateway.
+message PolicyTargetReference {


Can we give an example YAML referencing a Gateway? Its probably not obvious to most people

Added an AuthorizationPolicy example in selector.proto. Is this where you were looking for an example?

type/v1beta1/selector.proto

Signed-off-by: Jackie Elliott <[email protected]>

type/v1beta1/selector.proto

Signed-off-by: Jackie Elliott <[email protected]>

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Aug 8, 2023

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 8, 2023

jaellio commented Aug 8, 2023

View reviewed changes

networking/v1beta1/proxy_config.proto Outdated Show resolved Hide resolved

jaellio commented Aug 8, 2023

View reviewed changes

security/v1beta1/request_authentication.proto Outdated Show resolved Hide resolved

MorrisLaw reviewed Aug 9, 2023

View reviewed changes

type/target_ref.proto Outdated Show resolved Hide resolved

MorrisLaw reviewed Aug 9, 2023

View reviewed changes

howardjohn reviewed Aug 9, 2023

View reviewed changes

type/target_ref.proto Outdated Show resolved Hide resolved

type/target_ref.proto Outdated Show resolved Hide resolved

MorrisLaw approved these changes Aug 9, 2023

View reviewed changes

jaellio marked this pull request as ready for review August 9, 2023 17:59

jaellio requested a review from a team as a code owner August 9, 2023 17:59

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Aug 9, 2023

whitneygriffith reviewed Aug 9, 2023

View reviewed changes

extensions/v1alpha1/wasm.proto Outdated Show resolved Hide resolved

costinm reviewed Aug 10, 2023

View reviewed changes

whitneygriffith reviewed Aug 10, 2023

View reviewed changes

security/v1/authorization_policy.proto Outdated Show resolved Hide resolved

MorrisLaw mentioned this pull request Aug 10, 2023

Add status field to API #2900

Open

MorrisLaw mentioned this pull request Aug 8, 2023

Add targetRef field to Istio CRDs that use workloadSelector #2885

Closed

6 tasks

jaellio force-pushed the createTargetRefProto branch from 1cbbbe0 to 2383030 Compare August 11, 2023 19:06

linsun reviewed Aug 14, 2023

View reviewed changes

extensions/v1alpha1/wasm.proto Outdated Show resolved Hide resolved

linsun reviewed Aug 14, 2023

View reviewed changes

extensions/v1alpha1/wasm.proto Outdated Show resolved Hide resolved

linsun reviewed Aug 14, 2023

View reviewed changes

security/v1/authorization_policy.proto Outdated Show resolved Hide resolved

linsun reviewed Aug 14, 2023

View reviewed changes

security/v1/authorization_policy.proto Outdated Show resolved Hide resolved

linsun reviewed Aug 31, 2023

View reviewed changes

jaellio added 15 commits August 31, 2023 22:12

Define protobuf for PolicyTargetReference

08bf512

Signed-off-by: Jackie Elliott <[email protected]>

Add targetRef to AuthorizationPolicy, Telemetry, WasmPlugin,

2179f8f

ProxyConfig, and RequestAuthentication. Need more examples. Signed-off-by: Jackie Elliott <[email protected]>

Add examples

4898321

Signed-off-by: Jackie Elliott <[email protected]>

Moved targetRef def to selector.proto. Removed kubebuilder

035100f

comments. Added release note for targetRef. Signed-off-by: Jackie Elliott <[email protected]>

Add oneof to CRD protos. Add clarifying comments about intended

53184ca

use of taretRef. Signed-off-by: Jackie Elliott <[email protected]>

Remove targetRef from ProxyConfig

55f2999

Signed-off-by: Jackie Elliott <[email protected]>

Removed root namespace references and ingress gateway targetRef

6860eb1

examples. Signed-off-by: Jackie Elliott <[email protected]>

Hide API changes from docs and remove examples until impl is

c71a68a

complete Signed-off-by: Jackie Elliott <[email protected]>

Remove telemtry example until impl complete

db27003

Signed-off-by: Jackie Elliott <[email protected]>

add clarification resource must be in same ns as policy and add

73dfe72

oneof to wasm plugin. Signed-off-by: Jackie Elliott <[email protected]>

Remove oneof in to avoid go changes.

eb5b5e1

Signed-off-by: Jackie Elliott <[email protected]>

update release note to clarify scope is limited to waypoints

3e94b2a

Signed-off-by: Jackie Elliott <[email protected]>

Update authorizationPolicy selector comment

7acb929

Signed-off-by: Jackie Elliott <[email protected]>

clarify in targetRef description only waypoint is supported as a

0dc1482

targeted resource Signed-off-by: Jackie Elliott <[email protected]>

add k8s gateway references

ec70317

Signed-off-by: Jackie Elliott <[email protected]>

jaellio force-pushed the createTargetRefProto branch from 7144031 to ec70317 Compare August 31, 2023 22:13

jaellio requested a review from linsun August 31, 2023 22:35

linsun approved these changes Sep 5, 2023

View reviewed changes

howardjohn reviewed Sep 5, 2023

View reviewed changes

Respond to PR feedback and add selector example.

555ab53

Signed-off-by: Jackie Elliott <[email protected]>

keithmattix reviewed Sep 5, 2023

View reviewed changes

type/v1beta1/selector.proto Outdated Show resolved Hide resolved

type/v1beta1/selector.proto Outdated Show resolved Hide resolved

Address nits

1c9326a

Signed-off-by: Jackie Elliott <[email protected]>

howardjohn approved these changes Sep 5, 2023

View reviewed changes

istio-testing merged commit 283cc40 into istio:master Sep 5, 2023

jaellio mentioned this pull request May 11, 2024

Add jaellio as security maintainer istio/community#1399

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define targetRef proto #2888

Define targetRef proto #2888

jaellio commented Aug 8, 2023 •

edited

Loading

istio-testing commented Aug 8, 2023

istio-policy-bot commented Aug 8, 2023

jaellio commented Aug 8, 2023

MorrisLaw left a comment

MorrisLaw left a comment

MorrisLaw left a comment

costinm left a comment

hzxuzhonghu commented Aug 10, 2023

keithmattix commented Aug 10, 2023

hzxuzhonghu commented Aug 11, 2023

keithmattix commented Aug 11, 2023

hzxuzhonghu commented Aug 14, 2023

keithmattix commented Aug 14, 2023

linsun Aug 31, 2023 •

edited

Loading

jaellio Aug 31, 2023

linsun commented Aug 31, 2023

keithmattix commented Aug 31, 2023 •

edited

Loading

howardjohn commented Aug 31, 2023 via email

linsun commented Aug 31, 2023

howardjohn Sep 5, 2023

jaellio Sep 5, 2023

Define targetRef proto #2888

Define targetRef proto #2888

Conversation

jaellio commented Aug 8, 2023 • edited Loading

istio-testing commented Aug 8, 2023

istio-policy-bot commented Aug 8, 2023

jaellio commented Aug 8, 2023

MorrisLaw left a comment

Choose a reason for hiding this comment

MorrisLaw left a comment

Choose a reason for hiding this comment

MorrisLaw left a comment

Choose a reason for hiding this comment

costinm left a comment

Choose a reason for hiding this comment

hzxuzhonghu commented Aug 10, 2023

keithmattix commented Aug 10, 2023

hzxuzhonghu commented Aug 11, 2023

keithmattix commented Aug 11, 2023

hzxuzhonghu commented Aug 14, 2023

keithmattix commented Aug 14, 2023

linsun Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

jaellio Aug 31, 2023

Choose a reason for hiding this comment

linsun commented Aug 31, 2023

keithmattix commented Aug 31, 2023 • edited Loading

howardjohn commented Aug 31, 2023 via email

linsun commented Aug 31, 2023

howardjohn Sep 5, 2023

Choose a reason for hiding this comment

jaellio Sep 5, 2023

Choose a reason for hiding this comment

jaellio commented Aug 8, 2023 •

edited

Loading

linsun Aug 31, 2023 •

edited

Loading

keithmattix commented Aug 31, 2023 •

edited

Loading